Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add HydraContext #1581

Merged
merged 4 commits into from
May 4, 2021
Merged

Add HydraContext #1581

merged 4 commits into from
May 4, 2021

Conversation

jieru-hu
Copy link
Contributor

@jieru-hu jieru-hu commented Apr 28, 2021 •
edited by Jasha10
Loading

Plugin users

Upgrade the offending plugin, if it's still not resolved contact the plugin owner and ask for Hydra 1.1 support.

Plugin owners

This change is required to enable support for Hydra Callbacks and to remain compatible with future versions of Hydra. (Discussion doc).

The following apply to both Sweeper and Launcher plugins:

  • hydra_context is a required field for the setup function of these plugins and for run_job (typically called from Launchers)
  • config_loader is removed from Launcher, Sweeper's setup() signature

In particular, the setup() has been changed to

    def setup(
        self,
        *,
        hydra_context: HydraContext,
        task_function: TaskFunction,
        config: DictConfig,
    ) -> None:

run_job has been updated to

def run_job(
    task_function: TaskFunction,
    config: DictConfig,
    job_dir_key: str,
    job_subdir_key: Optional[str],
    configure_logging: bool = True,
    hydra_context: Optional[HydraContext] = None,
) -> "JobReturn":

For an example of how to upgrade the Plugin, check out the ExampleLauncher's setup and run_job call

Update 2021-01-13:

For Hydra 1.2, the hydra_context argument to run_job will no-longer be optional, as implemented in PR #1953.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 28, 2021
@jieru-hu jieru-hu force-pushed the 1498 branch 2 times, most recently from 3100fc6 to 265feaf Compare April 28, 2021 19:42
@jieru-hu jieru-hu requested a review from omry April 28, 2021 19:46
@jieru-hu jieru-hu marked this pull request as ready for review April 28, 2021 19:46
omry
omry reviewed Apr 28, 2021
View reviewed changes
Copy link
Collaborator

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking food.
See inline questions.

hydra/core/plugins.py Show resolved Hide resolved
hydra/core/utils.py Outdated Show resolved Hide resolved
plugins/hydra_ax_sweeper/hydra_plugins/hydra_ax_sweeper/_core.py
@@ -130,14 +130,15 @@ def __init__(self, ax_config: AxConfig, max_batch_size: Optional[int]):

def setup(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to release updates to all the sweepers and launchers when we make the first dev release including this.

plugins/hydra_ray_launcher/hydra_plugins/hydra_ray_launcher/_launcher_util.py
@@ -57,6 +60,7 @@ def launch_job_on_ray(
run_job_ray = ray.remote(_run_job)

ret = run_job_ray.remote(
hydra_context=hydra_context,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is different because the function did not take config_loader before.
Can you explain why it needs the hydra_context now?

This was referenced Apr 29, 2021
Closed
Closed
@jieru-hu
Copy link
Contributor Author

What is the error users will get if their launcher is not upgraded to the new signature?
We could potentially detect it and point them to a migration page.

Good point! This sounds better as a separate PR. I've created #1589 to track this.

hydra.types is potentially a better home for it (If placing it there does not introduce some hard circular dependencies).

moved!

We will need to release updates to all the sweepers and launchers when we make the first dev release including this.

yea, I've created #1590 to track this and assigned to myself.

This one is different because the function did not take config_loader before.
Can you explain why it needs the hydra_context now?

yep. run_job_ray decorates _run_job which actually calls run_job and as a result needs HydraContext.

@jieru-hu jieru-hu requested a review from omry April 29, 2021 18:56
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 29, 2021

This pull request introduces 2 alerts when merging 3e674d8 into c6b7143 - view on LGTM.com

new alerts:

  • 2 for Unused import

omry
omry requested changes Apr 30, 2021
View reviewed changes
Copy link
Collaborator

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change. Can you include an appropriate news fragment?

@jieru-hu jieru-hu marked this pull request as draft May 3, 2021 19:51
@jieru-hu
Copy link
Contributor Author

jieru-hu commented May 3, 2021

rebase and added a news fragment

@jieru-hu jieru-hu requested a review from omry May 3, 2021 20:13
@jieru-hu jieru-hu marked this pull request as ready for review May 3, 2021 20:13
@jieru-hu jieru-hu mentioned this pull request May 3, 2021
Merged
omry
omry approved these changes May 4, 2021
View reviewed changes
Copy link
Collaborator

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shipit

@jieru-hu jieru-hu merged commit c6397d2 into facebookresearch:master May 4, 2021
@jieru-hu jieru-hu deleted the 1498 branch May 4, 2021 00:40
@rsokl rsokl mentioned this pull request May 11, 2021
Merged
@Jasha10 Jasha10 mentioned this pull request Jan 11, 2022
Merged
@odelalleau odelalleau mentioned this pull request Jul 23, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omry omry omry approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jieru-hu @omry @facebook-github-bot